Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

melpomene: tracing improvements #2

Merged
merged 4 commits into from
Jul 7, 2022
Merged

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Jul 7, 2022

(This includes changes from #1 because that PR was opened from a fork
and I can't nicely stack PRs when one branch is on a fork)

This branch makes several small improvements to tracing in Melpomene:

  • melpomene: make tracing feature flags additive

    Currently, the tracing behavior in Melpomene is controlled by a
    pair of feature flags, "trace-modality" and "trace-fmt". These select
    between emitting traces to stdout using tracing_subscriber::fmt,
    and emitting to Modality using the tracing-modality crate. However,
    these feature flags are not additive. If they are both enabled, the
    code will not compile, since there are two implementations of the
    setup_tracing function, which are conditionally compiled based on
    whether those feature flags are enabled. If both are enabled, both
    functions will be present, resulting in a compiler error.

    This commit changes this code so that there is a single
    implementation of the setup_tracing function, which will construct
    a tracing subscriber that includes either fmt, Modality, or
    both, depending on the feature combination. Now, Melpomene should
    build even with --all-features or similar.

    Now that the fmt layer is built manually rather than using
    fmt::init() "easy mode" helper, I've also changed the filter
    environment variable from "RUST_LOG" to "MELPOMENE_TRACE". I thought
    this was nicer, but we can continue using the default env var if
    that's preferable.

  • melpomene: minor improvements to tracing configuration

  • melpomene: add some spans to the kernel/userspace threads

  • kernel: change the "initialized heap" event to format the addr in hex

Closes #1

hawkw added 4 commits July 7, 2022 09:20
Currently, the `tracing` behavior in Melpomene is controlled by a pair
of feature flags, "trace-modality" and "trace-fmt". These select between
emitting traces to stdout using `tracing_subscriber::fmt`, and emitting
to Modality using the `tracing-modality` crate. However, these feature
flags are not additive. If they are both enabled, the code will not
compile, since there are two implementations of the `setup_tracing`
function, which are conditionally compiled based on whether those
feature flags are enabled. If both are enabled, both functions will be
present, resulting in a compiler error.

This commit changes this code so that there is a single implementation
of the `setup_tracing` function, which will construct a `tracing`
subscriber that includes either `fmt`, Modality, or _both_, depending on
the feature combination. Now, Melpomene should build even with
`--all-features` or similar.

Now that the `fmt` layer is built manually rather than using
`fmt::init()` "easy mode" helper, I've also changed the filter
environment variable from "RUST_LOG" to "MELPOMENE_TRACE". I thought
this was nicer, but we can continue using the default env var if that's
preferable.
This branch makes the tracing configuration slightly nicer (IMO).
Depends on #1.
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw requested a review from jamesmunns July 7, 2022 16:46
@jamesmunns
Copy link
Contributor

Totally works for me! Thanks for the cleanup :)

@jamesmunns jamesmunns merged commit f910d34 into main Jul 7, 2022
@jamesmunns jamesmunns deleted the eliza/tracing-prettify branch July 17, 2022 00:01
jamesmunns added a commit that referenced this pull request Jun 4, 2023
containers: fix missing and wrong `Send` + `Sync` impls
jamesmunns added a commit that referenced this pull request Jun 4, 2023
These are the "non-`disk`" changes made for #2, without the actual "add the disk" support.

I'm opening this as a PR to hopefully allow @hawkw to merge it into her branches while I'm still working on disk branch.

Uncontroversial changes:

* Makes EntryKind non-exhaustive - could be used for robustness checking
* Adds a single method to allocation a dictionary entry, used to remove some copy/paste code
* adds `b@` and `b!` operators for byte-aligned (instead of cell-aligned) load/stores

Potentially controversial changes:

* Adds a `ptr_data: isize` union variant to the cell type, to be used when mixing "i32" data from the stack with "ptr" data from the stack
* ONLY `add` and `sub` actually use the `ptr_data` variant

At some point, we might want to pass and declare which operators ARE "pointer safe", and which ones aren't, or add specific ptr-safe operators. That being said, I'm not sure whether any operations OTHER than add or sub on a pointer are likely to be meaningful.

* Unrelated changes made during `disk` changes
* Update src/leakbox.rs
* Rename for consistency

Co-authored-by: Eliza Weisman <eliza@buoyant.io>
@jamesmunns jamesmunns added platform: melpomene Specific to the Melpomene simulator platform area: tools & build Related to host developer tools, including tracing, Crowtty and build processes labels Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: tools & build Related to host developer tools, including tracing, Crowtty and build processes platform: melpomene Specific to the Melpomene simulator platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants